Re: [PATCH 10/10] arm64: dts: amlogic: add the Amediatech X98Q

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] arm64: dts: amlogic: unimplemented amlogic,core-phase-degrees
  in x98q sd node
- [Medium] arm64: dts: amlogic: invalid rc-x98q map and wakeup-source on
  x98q ir node
- [Medium] arm64: dts: amlogic: unimplemented amlogic,shared-antenna on
  x98q bluetooth
- [Medium] arm64: dts: amlogic: x98q enables S4 usb, ge2d, and thermal
  without drivers
- [Low] arm64: dts: amlogic: unused sd_3v3 regulator claiming GPIOD_4 in
  x98q dts
--

commit 3b50767a39ee59efef6457c4aa95bbf033834748
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

arm64: dts: amlogic: add the Amediatech X98Q

This commit adds the device tree for the Amediatech X98Q TV box based on
the Amlogic S905W2 SoC.

> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4-s905w2-x98q.dts b/arch/arm64/boot/dts/amlogic/meson-s4-s905w2-x98q.dts
> new file mode 100644
> index 0000000000000..388198756a3a2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/amlogic/meson-s4-s905w2-x98q.dts

[ ... ]

> +	sd_3v3: regulator-sd-3v3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "SD_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio GPIOD_4 GPIO_ACTIVE_LOW>;
> +		regulator-always-on;
> +	};

[Severity: Low]
Should &sd use &sd_3v3 for vmmc-supply, or is regulator-sd-3v3 unused on
this board?

regulator-sd-3v3 claims GPIOD_4 and drives it low at boot via
regulator-always-on, but &sd below sets both vmmc-supply and vqmmc-supply
to &vddao_3v3, leaving &sd_3v3 unreferenced.

[ ... ]

> +&ir {
> +	pinctrl-0 = <&remote_pins>;
> +	pinctrl-names = "default";
> +	linux,rc-map-name = "rc-x98q";
> +	wakeup-source;
> +	status = "okay";
> +};

[Severity: Medium]
Are "rc-x98q" and wakeup-source supported on &ir?

"rc-x98q" is not in Documentation/devicetree/bindings/media/rc.yaml or
drivers/media/rc/keymaps/.  At probe, rc_prepare_rx_device() falls back to
RC_MAP_EMPTY (RC_PROTO_UNKNOWN) and passes that to
meson_ir_hw_decoder_init(), which returns early without setting
IR_DEC_REG1_ENABLE when RC_PROTO_BIT_NEC is not set, leaving the hardware
IR decoder disabled until reconfigured from userspace.

In addition, Documentation/devicetree/bindings/media/amlogic,meson6-ir.yaml
sets unevaluatedProperties: false and does not allow wakeup-source, and
drivers/media/rc/meson-ir.c does not configure wakeup support and disables
IR_DEC_REG1_ENABLE in meson_ir_suspend().

[ ... ]

> +&sd {
> +	pinctrl-0 = <&sdcard_pins>;
> +	pinctrl-1 = <&sdcard_clk_gate_pins>;
> +	pinctrl-names = "default", "clk-gate";
> +	bus-width = <4>;
> +	cap-sd-highspeed;
> +	/* Fails CRC at 50 MHz with the default 180 degree phase */
> +	amlogic,core-phase-degrees = <270>;
> +	max-frequency = <50000000>;

[Severity: High]
Does amlogic,core-phase-degrees have any effect in mainline?

Documentation/devicetree/bindings/mmc/amlogic,meson-gx-mmc.yaml sets
unevaluatedProperties: false and does not define
amlogic,core-phase-degrees, so dtbs_check fails.  At runtime,
meson_mmc_clk_init() in drivers/mmc/host/meson-gx-mmc.c unconditionally
programs CLK_PHASE_180 and never parses this property, leaving the SD
controller at the 180-degree phase where 50 MHz high-speed transfers fail
with CRC errors.

[ ... ]

> +&uart_a {
> +	pinctrl-0 = <&uart_a_pins>, <&uart_a_cts_rts_pins>;
> +	pinctrl-names = "default";
> +	uart-has-rtscts;
> +	status = "okay";
> +
> +	/* Bluetooth half of the Amlogic W1 combo chip */
> +	bluetooth {
> +		compatible = "amlogic,w155s2-bt";
> +		clocks = <&sdio_32k>;
> +		enable-gpios = <&gpio GPIOX_17 GPIO_ACTIVE_HIGH>;
> +		vddio-supply = <&vddio_ao1v8>;
> +		firmware-name = "amlogic/aml_w1_bt_uart.bin";
> +		amlogic,shared-antenna;
> +	};
> +};

[Severity: Medium]
Is amlogic,shared-antenna supported by the binding or driver for
amlogic,w155s2-bt?

Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
sets additionalProperties: false and does not define
amlogic,shared-antenna.  In drivers/bluetooth/hci_aml.c, aml_parse_dt()
does not read this property and aml_setup() passes
amldev->aml_dev_data->is_coex (false for data_w155s2) to aml_config_rf(),
configuring AML_RF_ANT_DOUBLE instead of AML_RF_ANT_SINGLE.

[ ... ]

> +&usb {
> +	status = "okay";
> +};
> +
> +&usb2_phy0 {
> +	status = "okay";
> +};
> +
> +&usb2_phy1 {
> +	status = "okay";
> +};

[Severity: Medium]
Can &usb, &ge2d, or GPU thermal throttling probe with the compatibles in
meson-s4.dtsi?

In arch/arm64/boot/dts/amlogic/meson-s4.dtsi, usb2_phy0 and usb2_phy1 use
compatible = "amlogic,s4-usb2-phy" without an "amlogic,g12a-usb2-phy"
fallback, while drivers/phy/amlogic/phy-meson-g12a-usb2.c only matches
"amlogic,g12a-usb2-phy" and "amlogic,a1-usb2-phy", causing
dwc3_meson_g12a_probe() to defer indefinitely in dwc3_meson_g12a_get_phys().
Also, usb2_phy0 and usb2_phy1 are already enabled in meson-s4.dtsi, so the
status overrides here are redundant.

Similarly, &ge2d uses "amlogic,s4-ge2d" without an "amlogic,axg-ge2d"
fallback (drivers/media/platform/amlogic/meson-ge2d/ge2d.c only matches
"amlogic,axg-ge2d"), and cpu_temp uses "amlogic,s4-thermal" without an
"amlogic,t7-thermal" fallback (drivers/thermal/amlogic_thermal.c only
matches "amlogic,t7-thermal"), so neither GE2D nor GPU thermal throttling
binds a driver despite being listed as supported in the commit message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106104659.54656-1-christian.koever-draxl@student.uibk.ac.at?part=10

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic