Re: [PATCH v2 06/13] thermal: tegra: Do not register cooling device

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]

On Fri, Oct 13, 2023 at 05:57:13PM +0200, Daniel Lezcano wrote:
> On 12/10/2023 19:58, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The SOCTHERM's built-in throttling mechanism doesn't map well to the
> > concept of a cooling device because it will automatically start to
> > throttle when the programmed temperature threshold is crossed.
> > 
> > Remove the cooling device implementation and instead unconditionally
> > program the throttling for the CPU and GPU thermal zones.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> 
> [ ... ]
> 
> > +	ret = of_property_read_u32(np, "temperature-millicelsius",
> > +				   &stc->temperature);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	ret = of_property_read_u32(np, "hysteresis-millicelsius",
> > +				   &stc->hysteresis);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	stc->num_zones = of_count_phandle_with_args(np, "nvidia,thermal-zones",
> > +						    NULL);
> > +	if (stc->num_zones > 0) {
> > +		struct device_node *zone;
> > +		unsigned int i;
> > +
> > +		stc->zones = devm_kcalloc(ts->dev, stc->num_zones, sizeof(zone),
> > +					  GFP_KERNEL);
> > +		if (!stc->zones)
> > +			return -ENOMEM;
> > +
> > +		for (i = 0; i < stc->num_zones; i++) {
> > +			zone = of_parse_phandle(np, "nvidia,thermal-zones", i);
> > +			stc->zones[i] = zone;
> > +		}
> > +	}
> 
> What is the connection between the temperature sensor and the hardware
> limiter?
> 
> I mean, one hand there is the hardware limiter which is not connected to the
> sensor neither a thermal zone and it could be self contained in a separate
> driver. And then there is the temperature sensor.
> 
> The thermal zone phandle things connected with the throttling bindings
> sounds like strange to me.
> 
> What prevents to split the throttling and the sensor into separate code?

Both the temperature sensor and the hardware throttle mechanism are part
of the same IP block, so it would be quite difficult (and unnecessary)
to split them into separate drivers.

The hardware throttler uses the temperature sensor's data to initiate
throttling automatically when certain (programmable) temperature
thresholds are reached.

The reason why we need to reference the thermal zone is because the
registers needed to program the throttler are contained within the
sensor group (which are effectively mapped to thermal zones).

I suppose there are a number of other ways how this could be described.
The thermal zones could be extended with extra information about the
throttling, or we could use just the sensor group ID instead of a full
phandle to reference this.

I was sort of trying to keep things somewhat aligned with the concept of
thermal zones and not rewrite the entire thing, but perhaps I should go
back to the drawing board and think about whether there's an even better
way to describe this in DT.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]