Re: [patch v1 1/2] dt-bindings: net: add binding documentation for mlxsw thermal control

Re: [patch v1 1/2] dt-bindings: net: add binding documentation for mlxsw thermal control

[Andrew Lunn]

> +- compatible		: "mellanox,mlxsw_minimal"

Interesting product name. Is there a mlxsw_maximal planned?

> +- reg			: The I2C address of the device.
> +
> +Optional properties:
> +- cooling-phandle	: phandle of the cooling device, which is to be used
> +			  for the zone thermal control.
> +			  If absent, cooling device controlled internally by
> +			  the ASIC may be used.
> +
> +- trips			: the nodes to describe a point in the temperature
> +			  domain with key temperatures at which cooling is
> +			  recommended. Each node must contain the next values:
> +			  - type: the trip type. Expected values are:
> +			    0 - a trip point to enable active cooling;
> +			    1 - a trip point to enable passive cooling;
> +			    2 - a trip point to notify emergency;
> +			  - temperature: unsigned integer indicating the trip
> +			    temperature level in millicelsius;
> +			  - minimum cooling state allowed within the trip node;
> +			  - maximum cooling state allowed within the trip node;
> +
> +Example:
> +	asic_thermal: mlxsw_minimal@48 {
> +		compatible = "mlxsw_minimal";

You missed the vendor part.

> +		reg = <0x48>;
> +		status = "disabled";

An example with it disabled?

> +		cooling-phandle = <&cooling>;
> +
> +		trips {
> +			trip@0 {
> +				trip = <0 75000 0 0>;
> +			};

I don't know much about the thermal subsystem. But looking at other
example binding documents, you seem to do something different here to
other drivers. Why do you not use what seems to be the common format:

               trips {
                        cpu_alert0: cpu-alert0 {
                                temperature = <90000>; /* millicelsius */
                                hysteresis = <2000>; /* millicelsius */
                                type = "active";
                        };
                        cpu_alert1: cpu-alert1 {
                                temperature = <100000>; /* millicelsius */
                                hysteresis = <2000>; /* millicelsius */
                                type = "passive";
                        };
                        cpu_crit: cpu-crit {
                                temperature = <125000>; /* millicelsius */
                                hysteresis = <2000>; /* millicelsius */
                                type = "critical";
                        };
                };

	Andrew

RE: [patch v1 1/2] dt-bindings: net: add binding documentation for mlxsw thermal control

[Vadim Pasternak]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew-g2DYL2Zd6BY@public.gmane.org]
> Sent: Tuesday, August 29, 2017 8:23 PM
> To: Vadim Pasternak <vadimp-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org;
> ivecera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [patch v1 1/2] dt-bindings: net: add binding documentation for
> mlxsw thermal control
> 
> > +- compatible		: "mellanox,mlxsw_minimal"
> 
> Interesting product name. Is there a mlxsw_maximal planned?
> 

Hi Andrew,

Thank you very much for review.

No plans for such product. We just have fully functional drivers for different
kind of Mellanox switch devices like spectrum, switchib, switchx2. All of them
work over PCI bus. The minimal is supposed to be used for the chassis
management and we uses it at BMC side. It works over I2C bus and doesn't
depend on switch type. So it has a minaml functionality, so name "minimal".

> > +- reg			: The I2C address of the device.
> > +
> > +Optional properties:
> > +- cooling-phandle	: phandle of the cooling device, which is to be used
> > +			  for the zone thermal control.
> > +			  If absent, cooling device controlled internally by
> > +			  the ASIC may be used.
> > +
> > +- trips			: the nodes to describe a point in the
> temperature
> > +			  domain with key temperatures at which cooling is
> > +			  recommended. Each node must contain the next
> values:
> > +			  - type: the trip type. Expected values are:
> > +			    0 - a trip point to enable active cooling;
> > +			    1 - a trip point to enable passive cooling;
> > +			    2 - a trip point to notify emergency;
> > +			  - temperature: unsigned integer indicating the trip
> > +			    temperature level in millicelsius;
> > +			  - minimum cooling state allowed within the trip
> node;
> > +			  - maximum cooling state allowed within the trip
> node;
> > +
> > +Example:
> > +	asic_thermal: mlxsw_minimal@48 {
> > +		compatible = "mlxsw_minimal";
> 
> You missed the vendor part.

Acked.

> 
> > +		reg = <0x48>;
> > +		status = "disabled";
> 
> An example with it disabled?

We just use it in such way at BMC side. It's disabled by default and upon
event indicating the good health for the device the device driver is
connected. I can remove it from the example. But for BMC it's actually
the default state.

> 
> > +		cooling-phandle = <&cooling>;
> > +
> > +		trips {
> > +			trip@0 {
> > +				trip = <0 75000 0 0>;
> > +			};
> 
> I don't know much about the thermal subsystem. But looking at other
> example binding documents, you seem to do something different here to
> other drivers. Why do you not use what seems to be the common format:

In mlxsw_thermal driver we have definition for the thermal trips, which contains
the type, like  "active" of "passive", temperature  in millicelsius and min/max states
for cooling device. These vector defines thermal trip points.
The hysteresis parameter is not relevant.

For example, ASIC thermal sensor is associated with the cooling device like:
&pwm_tacho {
...
	cooling: fan@0 {
		reg = <0x00>;
		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
		aspeed,fan-tach-ch = /bits/ 8 <0x00>;
	};

And the below sub-nodes
			trip@0 {
				trip = <0 75000 0 0>;
			};
			trip@1 {
				trip = <2 85000 1 5>;
			};
			trip@3 {
				trip = <2 105000 5 5>;
			};

defines that PWM should be at default speed (125), while temperature is
below 75000, should be at max speed (255), while temperature is above 
10500, and should step according the temperate trend between.

Thanks,
Vadim.

> 
>                trips {
>                         cpu_alert0: cpu-alert0 {
>                                 temperature = <90000>; /* millicelsius */
>                                 hysteresis = <2000>; /* millicelsius */
>                                 type = "active";
>                         };
>                         cpu_alert1: cpu-alert1 {
>                                 temperature = <100000>; /* millicelsius */
>                                 hysteresis = <2000>; /* millicelsius */
>                                 type = "passive";
>                         };
>                         cpu_crit: cpu-crit {
>                                 temperature = <125000>; /* millicelsius */
>                                 hysteresis = <2000>; /* millicelsius */
>                                 type = "critical";
>                         };
>                 };
> 
> 	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch v1 0/2] add support for the external thermal zone and cooling device binding for Mellanox network devices

[Vadim Pasternak]

It allows binding of AISC thermal sensor to externally defined thermal
zones and cooling device. Such definition can be provided from DTS.

Vadim Pasternak (2):
  dt-bindings: net: add binding documentation for mlxsw thermal control
  mlxsw: core: add support for the external thermal zone setting (by
    DTS)

 .../devicetree/bindings/net/mellanox,mlxsw.txt     |  46 +++++++++
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 107 ++++++++++++++++++++-
 drivers/net/ethernet/mellanox/mlxsw/minimal.c      |   6 ++
 3 files changed, 155 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/mellanox,mlxsw.txt

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch v1 1/2] dt-bindings: net: add binding documentation for mlxsw thermal control

[Vadim Pasternak]

Add binding document for Mellanox switch devices.

Signed-off-by: Vadim Pasternak <vadimp-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 .../devicetree/bindings/net/mellanox,mlxsw.txt     | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mellanox,mlxsw.txt

diff --git a/Documentation/devicetree/bindings/net/mellanox,mlxsw.txt b/Documentation/devicetree/bindings/net/mellanox,mlxsw.txt
new file mode 100644
index 0000000..55de5ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mellanox,mlxsw.txt
@@ -0,0 +1,46 @@
+Mellanox Technologies Switch ASICs
+
+This file provides information, what the device node
+for the Switch ASIC interface contains.
+
+Required properties:
+- compatible		: "mellanox,mlxsw_minimal"
+- reg			: The I2C address of the device.
+
+Optional properties:
+- cooling-phandle	: phandle of the cooling device, which is to be used
+			  for the zone thermal control.
+			  If absent, cooling device controlled internally by
+			  the ASIC may be used.
+
+- trips			: the nodes to describe a point in the temperature
+			  domain with key temperatures at which cooling is
+			  recommended. Each node must contain the next values:
+			  - type: the trip type. Expected values are:
+			    0 - a trip point to enable active cooling;
+			    1 - a trip point to enable passive cooling;
+			    2 - a trip point to notify emergency;
+			  - temperature: unsigned integer indicating the trip
+			    temperature level in millicelsius;
+			  - minimum cooling state allowed within the trip node;
+			  - maximum cooling state allowed within the trip node;
+
+Example:
+	asic_thermal: mlxsw_minimal@48 {
+		compatible = "mlxsw_minimal";
+		reg = <0x48>;
+		status = "disabled";
+		cooling-phandle = <&cooling>;
+
+		trips {
+			trip@0 {
+				trip = <0 75000 0 0>;
+			};
+			trip@1 {
+				trip = <2 85000 1 5>;
+			};
+			trip@3 {
+				trip = <2 105000 5 5>;
+			};
+		};
+	};
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch v1 2/2] mlxsw: core: add support for the external thermal zone setting (by DTS)

[Vadim Pasternak]

It allows:
- thermal zone setting for mlxsw based HW and from DTS file;
- binding ASIC temperature sensor to  cooling devices.
It requires setting of CONFIG_OF.

Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 107 ++++++++++++++++++++-
 drivers/net/ethernet/mellanox/mlxsw/minimal.c      |   6 ++
 2 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
index d866c98..c30783e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
@@ -34,6 +34,7 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/device.h>
+#include <linux/of_platform.h>
 #include <linux/sysfs.h>
 #include <linux/thermal.h>
 #include <linux/err.h>
@@ -44,6 +45,7 @@
 #define MLXSW_THERMAL_MAX_TEMP	110000	/* 110C */
 #define MLXSW_THERMAL_MAX_STATE	10
 #define MLXSW_THERMAL_MAX_DUTY	255
+#define MLXSW_THERMAL_TRIP_ELEM	4
 
 struct mlxsw_thermal_trip {
 	int	type;
@@ -98,6 +100,8 @@ struct mlxsw_thermal {
 	struct thermal_cooling_device *cdevs[MLXSW_MFCR_PWMS_MAX];
 	struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS];
 	enum thermal_device_mode mode;
+	int ntrips;
+	bool cooling_external;
 };
 
 static inline u8 mlxsw_state_to_duty(int state)
@@ -121,6 +125,10 @@ static int mlxsw_get_cooling_device_idx(struct mlxsw_thermal *thermal,
 		if (thermal->cdevs[i] == cdev)
 			return i;
 
+	/* Allow external cooling binding if theres is no local. */
+	if (thermal->cooling_external)
+		return 0;
+
 	return -ENODEV;
 }
 
@@ -334,6 +342,83 @@ static const struct thermal_cooling_device_ops mlxsw_cooling_ops = {
 	.set_cur_state	= mlxsw_thermal_set_cur_state,
 };
 
+#ifdef CONFIG_OF
+static int
+mlxsw_thermal_of_init(struct device *dev, struct mlxsw_thermal *thermal)
+{
+	struct device_node *np = dev->of_node;
+	u32 trip[MLXSW_THERMAL_TRIP_ELEM];
+	struct platform_device *pdev;
+	struct device_node *phandle;
+	struct device_node *gchild;
+	struct device_node *child;
+	int ntrips;
+	int i;
+	int ret;
+
+	/* trips */
+	child = of_get_child_by_name(np, "trips");
+
+	/* No trips provided */
+	if (!child) {
+		of_node_put(child);
+		return -EINVAL;
+	}
+
+	/* Cooling device is optional parameter. If it is not defined, driver
+	 * will try to connect PWM which is owned, if any.
+	 */
+	phandle = of_parse_phandle(child, "cooling-phandle", 0);
+	if (phandle) {
+		pdev = of_find_device_by_node(phandle);
+		of_node_put(phandle);
+		if (!pdev) {
+			ret = -ENODEV;
+			goto put_child;
+		}
+
+		thermal->cooling_external = true;
+	}
+
+	ntrips = of_get_child_count(child);
+	if (ntrips == 0) {
+		/* should have at least one child */
+		ret = 0;
+		goto put_child;
+	}
+
+	i = 0;
+	for_each_child_of_node(child, gchild) {
+		ret = of_property_count_u32_elems(gchild, "trip");
+		if (ret != MLXSW_THERMAL_TRIP_ELEM) {
+			ret = -EINVAL;
+			goto put_child;
+		}
+
+		ret = of_property_read_u32_array(gchild, "trip",
+						 trip, ret);
+		if (ret)
+			goto put_gchild;
+
+		memcpy(&thermal->trips[i++], trip, sizeof(trip));
+	}
+	ret = ntrips;
+
+put_gchild:
+	of_node_put(gchild);
+put_child:
+	of_node_put(child);
+
+	return ret;
+}
+#else
+static int
+mlxsw_thermal_of_init(struct device *dev, struct mlxsw_thermal *thermal)
+{
+	return 0;
+}
+#endif
+
 int mlxsw_thermal_init(struct mlxsw_core *core,
 		       const struct mlxsw_bus_info *bus_info,
 		       struct mlxsw_thermal **p_thermal)
@@ -344,7 +429,9 @@ int mlxsw_thermal_init(struct mlxsw_core *core,
 	struct mlxsw_thermal *thermal;
 	u16 tacho_active;
 	u8 pwm_active;
-	int err, i;
+	int ntrips;
+	int i;
+	int err;
 
 	thermal = devm_kzalloc(dev, sizeof(*thermal),
 			       GFP_KERNEL);
@@ -353,7 +440,19 @@ int mlxsw_thermal_init(struct mlxsw_core *core,
 
 	thermal->core = core;
 	thermal->bus_info = bus_info;
-	memcpy(thermal->trips, default_thermal_trips, sizeof(thermal->trips));
+
+	if (dev->of_node) {
+		ntrips = mlxsw_thermal_of_init(dev, thermal);
+		if (ntrips > 0)
+			thermal->ntrips = ntrips;
+	}
+
+	if (!dev->of_node || ntrips <= 0) {
+		/* Use default if the external setting is not available */
+		memcpy(thermal->trips, default_thermal_trips,
+		       sizeof(thermal->trips));
+		thermal->ntrips = MLXSW_THERMAL_NUM_TRIPS;
+	}
 
 	err = mlxsw_reg_query(thermal->core, MLXSW_REG(mfcr), mfcr_pl);
 	if (err) {
@@ -398,8 +497,8 @@ int mlxsw_thermal_init(struct mlxsw_core *core,
 	}
 
 	thermal->tzdev = thermal_zone_device_register("mlxsw",
-						      MLXSW_THERMAL_NUM_TRIPS,
-						      MLXSW_THERMAL_TRIP_MASK,
+						      thermal->ntrips,
+						      BIT(thermal->ntrips) - 1,
 						      thermal,
 						      &mlxsw_thermal_ops,
 						      NULL, 0,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/minimal.c b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
index 3dd1626..8e3cc13 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/minimal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
@@ -56,8 +56,14 @@ static const struct i2c_device_id mlxsw_minimal_i2c_id[] = {
 	{ },
 };
 
+static const struct of_device_id mlxsw_minimal_dt_match[] = {
+	{ .compatible = "mellanox,mlxsw_minimal" },
+	{ },
+};
+
 static struct i2c_driver mlxsw_minimal_i2c_driver = {
 	.driver.name = "mlxsw_minimal",
+	.driver.of_match_table = of_match_ptr(mlxsw_minimal_dt_match),
 	.class = I2C_CLASS_HWMON,
 	.id_table = mlxsw_minimal_i2c_id,
 };
-- 
2.1.4

Re: [patch v1 1/2] dt-bindings: net: add binding documentation for mlxsw thermal control

[Andrew Lunn]

On Tue, Aug 29, 2017 at 05:57:01PM +0000, Vadim Pasternak wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew-g2DYL2Zd6BY@public.gmane.org]
> > Sent: Tuesday, August 29, 2017 8:23 PM
> > To: Vadim Pasternak <vadimp-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org;
> > ivecera-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [patch v1 1/2] dt-bindings: net: add binding documentation for
> > mlxsw thermal control
> > 
> > > +- compatible		: "mellanox,mlxsw_minimal"
> > 
> > Interesting product name. Is there a mlxsw_maximal planned?
> > 
> 
> Hi Andrew,
> 
> Thank you very much for review.
> 
> No plans for such product. We just have fully functional drivers for different
> kind of Mellanox switch devices like spectrum, switchib, switchx2. All of them
> work over PCI bus. The minimal is supposed to be used for the chassis
> management and we uses it at BMC side. It works over I2C bus and doesn't
> depend on switch type. So it has a minaml functionality, so name "minimal".

That is not really how device tree compatible names work. The
compatible should be the product name, or the first product which had
this functionality. So in this case is the product is the chassis?
What name does the chassis have?

At some point, it would be good if you made the .dts file public. That
would give people a better idea how this all fits together.

> > I don't know much about the thermal subsystem. But looking at other
> > example binding documents, you seem to do something different here to
> > other drivers. Why do you not use what seems to be the common format:
> 
> In mlxsw_thermal driver we have definition for the thermal trips, which contains
> the type, like  "active" of "passive", temperature  in millicelsius and min/max states
> for cooling device. These vector defines thermal trip points.
> The hysteresis parameter is not relevant.

As i said, i don't know much about the thermal subsystem. So i cannot
comment on if your binding is good or bad. It just seems different,
which is often bad.

You really need the thermal subsystem guys to review this. But you
didn't Cc: them.

./scripts/get_maintainer.pl drivers/thermal/thermal_core.c 
Zhang Rui <rui.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> (supporter:THERMAL)
Eduardo Valentin <edubezval-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> (supporter:THERMAL)
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:THERMAL)

This driver has nothing to do with netdev. I'm wondering if it
actually belongs in drivers/thermal? Or maybe drivers/hwmon?
The pm and hwmon guys will probably tell you.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html