Re: [PATCH v5 3/3] arm64: dts: qcom: Add base qcs6490-rb3gen2 board dts

[Caleb Connolly]

On 20/11/2023 13:47, Komal Bajaj wrote:
> Add DTS for Qualcomm qcs6490-rb3gen2 board which uses
> QCS6490 SoC. This adds debug uart and usb support along
> with regulators found on this board.

Hi,

I understand there was a lot of previous discussion around these two
boards, sorry to be bringing it up again here, but I have a few more
questions.

How similar are these two boards in terms of design? If they're derived
from the same reference schematic then I think this is a good
justification to de-duplicate the common DTS parts.

Dropping them in a diff tool [1] it seems as though the only changes are
the modem reserved memory for the IDP board, some minor regulator
changes, and the sdcard on the IDP board being enabled. However it's
important to differentiate between these just, being the same, vs them
being based on the same reference design.

I left some comments on the parts that differ between the boards below,
but basically my question is: do these boards share enough of the same
*design* that it would make sense to have a "qcm6490-iot.dtsi" file with
the common reserved memory and regulators?

The IDP and rb3 boards would then inherit from there, avoiding a lot of
duplication and weirdness where some boards have certain regulator
properties that others don't with it being hard to tell if this is
intentional or not (this is the case with a lot of the existing upstream
devices).

On a related note, should we further split the rb3 board into a
qcs6490-whatever-som.dtsi file which may define the SoM specific parts?
This would undoubtebly make it easier for other boards based on the same
SoM to be bought up and kept up to date.

[1]: https://quickdiff.net/?unique_id=630F6851-C750-839E-1651-4CA6D997A74D


[...]

> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> new file mode 100644
> index 000000000000..f023dcf768f1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
[...]
> +
> +		vreg_l7b_2p952: ldo7 {

The IDP board defined voltages here for the sdcard, does the rb3 board
have an sdcard slot (if so which regulator does it use)?

Is there a reason not to define the same voltage range for this board?
> +			regulator-allow-set-load;

This property is set for rb3 but not for the idp board, even though this
regulator is unused, should this be set?
> +			regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +
[...]
> +
> +		vreg_l9b_1p2: ldo9 {
Same question as above
> +			regulator-allow-set-load;
Same question
> +			regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM RPMH_REGULATOR_MODE_HPM>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +
[...]
> +		vreg_l19b_1p8: ldo19 {
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <2000000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
The IDP board has the regulator-allow-set-load property here, as well as
regulator-allowed-modes. This regulator is used for the sdcard on that
board. Is it used for anything on rb3? Can these properties be the same?
> +		};
> +	};
> +


Thanks and regards,
-- 
// Caleb (they/them)