Re: [PATCH 2/2] arm64: dts: qcom: sm7225: Add Samsung Galaxy Tab S7 FE LTE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] arm64: dts: qcom: missing PMK8350_SID causes SPMI SID 0
  collision
- [Medium] arm64: dts: qcom: swapped vdda-phy and vdda-pll supplies on
  usb_1_qmpphy
- [Medium] arm64: dts: qcom: broken chosen stdout-path and missing
  display AXI clock
- [Low] arm64: dts: qcom: missing sdc2 card-detect pinctrl and duplicate
  ipa props
- [Low] arm64: dts: qcom: undeleted cont_splash_memory and knox_reserved
  mismatch
--

commit 40269d8494daca48cf8a14a66c04f07d121893ba
Author: Vishal Balasubramanian <vishal.bala.nps@gmail.com>

arm64: dts: qcom: sm7225: Add Samsung Galaxy Tab S7 FE LTE

This commit adds the initial device tree for the Samsung Galaxy Tab S7 FE
LTE (SM-T735) tablet based on the Qualcomm SM7225 SoC.

> diff --git a/arch/arm64/boot/dts/qcom/sm7225-samsung-gts7xllite.dts b/arch/arm64/boot/dts/qcom/sm7225-samsung-gts7xllite.dts
> new file mode 100644
> index 0000000000000..f3e3529277414
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm7225-samsung-gts7xllite.dts
> @@ -0,0 +1,575 @@

[ ... ]

> +#include "sm7225.dtsi"
> +#include "pm6150l.dtsi"
> +#include "pm6350.dtsi"
> +#include "pmk8350.dtsi"

[Severity: High]
Does including pmk8350.dtsi without defining PMK8350_SID to 6 cause pmk8350
to collide with pm6350 on SPMI USID 0?

pmk8350.dtsi defaults PMK8350_SID to 0, so pmk8350: pmic@PMK8350_SID expands
to pmic@0 and merges into the PM6350 node from pm6350.dtsi.  That overwrites
pmic@0's compatible string with "qcom,pmk8350", places all PMK8350
sub-devices (such as pmk8350_rtc, pmk8350_vadc, and the reboot-reason nvmem
cell in pmk8350_sdam_2) on USID 0 instead of USID 6, and leaves pmk8350_pon
enabled alongside pm6350_pon at 0x800.  Should this define PMK8350_SID to 6
before including pmk8350.dtsi and disable or delete &pmk8350_pon, as done in
sm7225-fairphone-fp4.dts?

[ ... ]

> +	aliases {
> +		serial0 = &uart9;
> +		serial1 = &uart1;
> +	};
> +
> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		stdout-path = "framebuffer0";
> +
> +		framebuffer0: framebuffer {
> +			compatible = "simple-framebuffer";
> +			memory-region = <&framebuffer_mem>;
> +			width = <1600>;
> +			height = <2560>;
> +			stride = <(1600 * 4)>;
> +			format = "a8r8g8b8";
> +		};
> +	};

[Severity: Medium]
Can early_init_dt_scan_chosen_stdout() resolve stdout-path = "framebuffer0",
and will clk_disable_unused() gate the display AXI clock?

Because "framebuffer0" does not start with '/', of_find_node_opts_by_path()
looks it up as a property in /aliases, which only defines serial0 and
serial1, leaving of_stdout NULL; using stdout-path = &framebuffer0; (or
"/chosen/framebuffer") resolves to the node path.  Also, because &mdss is
disabled and gcc_disp_axi_clk is not marked CLK_IS_CRITICAL in gcc-sm6350.c,
omitting clocks = <&gcc GCC_DISP_AXI_CLK> from the simple-framebuffer node
leaves the display AXI clock unclaimed when unused clocks are disabled.
Finally, #address-cells, #size-cells, and ranges in /chosen are unused since
the framebuffer node uses memory-region rather than reg.

[ ... ]

> +		framebuffer_mem: memory@a0000000 {
> +			reg = <0 0xa0000000 0 (2560 * 1600 * 4)>;
> +			no-map;
> +		};
> +
> +		knox_reserved: memory@b0200000 {
> +			reg = <0x0 0xb0200000 0x0 0x200000>;
> +			no-map;
> +		};

[Severity: Low]
Should cont_splash_memory be deleted with /delete-node/ before redeclaring
memory@a0000000 as framebuffer_mem, as done for the other replaced
reserved-memory nodes at the top of the file?  Also, the commit message
describes the extra bootloader reservation as the fastuh region, whereas this
node is labeled knox_reserved.

[ ... ]

> +&ipa {
> +	qcom,gsi-loader = "self";
> +	memory-region = <&pil_ipa_fw_mem>;
> +	firmware-name = "qcom/sm7225/Samsung/gts7xllite/lagoon_ipa_fws.mbn";
> +
> +	status = "okay";
> +};

[ ... ]

> +&sdhc_2 {
> +	vmmc-supply = <&vreg_l9e>;
> +	vqmmc-supply = <&vreg_l6e>;
> +
> +	cd-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
> +
> +	status = "okay";
> +};

[Severity: Low]
Does sdhc_2 need pin configuration for the gpio94 card-detect line in
&sdc2_on_state and &sdc2_off_state?

sm6350.dtsi only configures clk, cmd, and data in sdc2_on_state and
sdc2_off_state, leaving gpio94 without pull-up and sleep bias settings (see
sd-cd-pins in sm7225-fairphone-fp4.dts).  Also, qcom,gsi-loader and
memory-region in &ipa duplicate the properties already set in sm6350.dtsi.

[ ... ]

> +&usb_1_qmpphy {
> +	vdda-phy-supply = <&vreg_l22a>;
> +	vdda-pll-supply = <&vreg_l16a>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Are vdda-phy-supply and vdda-pll-supply swapped on usb_1_qmpphy?

vreg_l16a is the ~0.88V rail and vreg_l22a is the ~1.2V rail (used for
vdda-pll-supply on ufs_mem_phy above).  On SM6350/SM7225 QMP PHYs (such as
sm7225-fairphone-fp4.dts and sm6350-sony-xperia-lena-pdx213.dts),
vdda-phy-supply is <&vreg_l16a> and vdda-pll-supply is <&vreg_l22a>; swapping
them applies the PHY core load vote to the 1.2V rail and the PLL load vote to
the 0.88V rail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-master-v1-0-07b4f06a9dd5@gmail.com?part=2