Re: [PATCH v4 3/4] arm64: dts: qcom: sc8280xp: add Radxa Dragon Q8B

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: unpopulated ADC7_AMUX_THM5 channel breaks adp12v_voltage
- [Medium] arm64: dts: qcom: invalid trip2 in pmc8280c-thermal breaks CPU throttling
--

commit 1587f072e6febbe214d258d0ce37a60770079813
Author: Xilin Wu <sophon@radxa.com>

arm64: dts: qcom: sc8280xp: add Radxa Dragon Q8B

This commit adds initial device tree support for the Radxa Dragon Q8B
single-board computer based on the Qualcomm Snapdragon 8cx Gen 3 SC8280XP
platform.

> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-radxa-dragon-q8b.dts b/arch/arm64/boot/dts/qcom/sc8280xp-radxa-dragon-q8b.dts
> new file mode 100644
> index 0000000000000..e0fae337ab667
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-radxa-dragon-q8b.dts

[ ... ]

> +	thermal-zones {

[ ... ]

> +		pmc8280c-thermal {
> +			cooling-maps {
> +				map0 {
> +					trip = <&pmc8280c_alert0>;
> +					/* Throttle X1C cores to 2496MHz at 105C */
> +					cooling-device = <&cpu4 5 5>,
> +							 <&cpu5 5 5>,
> +							 <&cpu6 5 5>,
> +							 <&cpu7 5 5>;
> +				};
> +			};
> +
> +			trips {
> +				pmc8280c_alert0: trip2 {
> +					temperature = <105000>;
> +					hysteresis = <0>;
> +					type = "passive";
> +				};
> +			};
> +		};

[Severity: Medium]
Can this trip2 node in pmc8280c-thermal work as intended alongside the base
definition in arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi?

In sc8280xp-pmics.dtsi, pmc8280c_thermal already defines two trips on
pmc8280c_temp_alarm (which has no io-channels property):

	pmc8280c_thermal: pmc8280c-thermal {
		polling-delay-passive = <100>;

		thermal-sensors = <&pmc8280c_temp_alarm>;

		trips {
			trip0 {
				temperature = <95000>;
				hysteresis = <0>;
				type = "passive";
			};

			trip1 {
				temperature = <115000>;
				hysteresis = <0>;
				type = "critical";
			};
		};
	};

Appending trip2 at 105000 mC results in three trips ordered as 95000, 115000,
and 105000. In drivers/thermal/qcom/qcom-spmi-temp-alarm.c:

1. On GEN2 rev2 hardware, qpnp_tm_init() ->
   qpnp_tm_gen2_rev2_configure_trip_temps() programs the three hardware stages
   in DTB order and then enforces strictly increasing thresholds:

	for (i = 1; i < STAGE_COUNT; i++) {
		if (chip->temp_thresh_map[i] <= chip->temp_thresh_map[i - 1]) {
			dev_err(chip->dev, "Threshold %d=%ld <= threshold %d=%ld\n",
				i, chip->temp_thresh_map[i], i - 1,
				chip->temp_thresh_map[i - 1]);
			return -EINVAL;
		}
	}

   Because threshold 2 (105000) is less than threshold 1 (115000), probe
   fails with -EINVAL.

2. On GEN2 rev0/rev1 hardware without an ADC channel, qpnp_tm_get_temp() ->
   qpnp_tm_update_temp_no_adc() only reports discrete stage temperatures
   (37000 mC in Stage 0, 97000 mC in Stage 1, and 117000 mC in Stage 2):

	if (stage_new > stage_old) {
		/* increasing stage, use lower bound */
		chip->temp = qpnp_tm_decode_temp(chip, stage_new)
				+ TEMP_STAGE_HYSTERESIS;
	}

   As temperature rises, the reported value jumps directly from 97000 mC to
   117000 mC (crossing trip1 at 115000 mC), so trip2 at 105000 mC and its CPU
   cooling map will never activate before shutdown.

Should pmc8280c-thermal override trip0 (or wire an ADC channel to
pmc8280c_temp_alarm) instead of adding trip2?

[ ... ]

> +	adp12v_voltage: voltage-divider-adp12v {
> +		compatible = "voltage-divider";
> +		io-channels = <&pmk8280_vadc PM8350_ADC7_AMUX_THM5(1)>;
> +		#io-channel-cells = <1>;
> +
> +		/* R470 (110K) in series with R471 (10K) to ground. */
> +		full-ohms = <120000>;
> +		output-ohms = <10000>;
> +	};

[ ... ]

> +&pmk8280_vadc {
> +	channel@108 {
> +		reg = <PM8350_ADC7_AMUX_THM5(1)>;
> +		qcom,hw-settle-time = <200>;
> +		qcom,pre-scaling = <1 1>;
> +		label = "adp12v_det";
> +	};

[Severity: Medium]
Will channel@108 and its consumers adp12v_voltage and iio-hwmon fail to probe
at runtime when using PM8350_ADC7_AMUX_THM5(1)?

In drivers/iio/adc/qcom-spmi-adc5.c, pmk8280_vadc (compatible "qcom,spmi-adc7")
uses adc7_chans_pmic[]. That array defines [ADC7_AMUX_THM5_100K_PU] (0x48),
but has no entry for unpulled [ADC7_AMUX_THM5] (0x08):

	static const struct adc5_channels adc7_chans_pmic[ADC5_MAX_CHANNEL] = {
		[ADC7_REF_GND]		= ADC5_CHAN_VOLT("ref_gnd", 0,
						SCALE_HW_CALIB_DEFAULT)
		...
		[ADC7_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_pu2", 0,
						SCALE_HW_CALIB_THERM_100K_PU_PM7)

When adc5_get_fw_data() initializes channel@108 from adc7_chans_pmic[0x08],
the zero-initialized entry leaves iio_chan->info_mask_separate as 0.

When drivers/iio/afe/iio-rescale.c probes adp12v_voltage via rescale_probe() ->
rescale_configure_channel(), it checks the source channel capabilities:

	if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) &&
	    (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE) ||
	     iio_channel_has_info(schan, IIO_CHAN_INFO_OFFSET))) {
		dev_info(dev, "using raw+scale/offset source channel\n");
	} else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) {
		dev_info(dev, "using processed channel\n");
		rescale->chan_processed = true;
	} else {
		dev_err(dev, "source channel is not supported\n");
		return -EINVAL;
	}

Because neither IIO_CHAN_INFO_RAW nor IIO_CHAN_INFO_PROCESSED is set on the
source channel, adp12v_voltage fails to probe with -EINVAL, and the
downstream iio-hwmon node defers probe indefinitely.

Does adc7_chans_pmic[] in drivers/iio/adc/qcom-spmi-adc5.c need an
ADC7_AMUX_THM5 voltage channel entry before this channel can be used here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-q8b-dts-v4-0-e8e7438f5545@radxa.com?part=3