On Fri, 2026-01-09 at 13:59 -0600, Nishanth Menon wrote:
> On 09:14-20260107, Matthias Schiffer wrote:
> > The TQMa62xx is a SoM family with a pluggable board connector based on the
> > TI AM62x SoCs. Add DTS(I) for the AM625 (2x Cortex-A53) variant and its
> > combination with our MBa62xx carrier board.
> >
> > Signed-off-by: Matthias Schiffer <[email protected]>
>
> Few clarifications below..
>
> [...]
> > diff --git a/arch/arm64/boot/dts/ti/k3-am625-tqma62xx-mba62xx.dts
> > b/arch/arm64/boot/dts/ti/k3-am625-tqma62xx-mba62xx.dts
> > new file mode 100644
> > index 0000000000000..cca8d0018504d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/ti/k3-am625-tqma62xx-mba62xx.dts
> > @@ -0,0 +1,930 @@
>
> [...]
>
> > + backlight: backlight {
> > + compatible = "pwm-backlight";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&backlight_pins>;
> > + enable-gpios = <&main_gpio0 38 GPIO_ACTIVE_HIGH>;
> > + status = "disabled";
>
> Document why disabled?
Will be enabled by DTSO for different panels. This will be added in a follow-up
series, so I'll add a comment.
>
> [...]
> > +
> > + panel: panel {
> > + pinctrl-names = "default";
>
> Missing compatible? thinking again.. Should'nt these be part of some
> panel dtsi?
We tried to leave as much common configuration for different panels as possible
in the base DTS, so the DTBOs aren't bigger than necessary. I think it makes
sense to have this in the board DTS, as the signals used by the panels are part
of the board design.
>
> > + pinctrl-0 = <&lvds_panel_pins>;
> > + enable-gpios = <&main_gpio0 36 GPIO_ACTIVE_HIGH>;
> > + power-supply = <®_lvds_pwr>;
> > + };
> > +
> > + fan0: pwm-fan {
> > + compatible = "pwm-fan";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pwm_fan_pins>;
> > + fan-supply = <®_pwm_fan>;
> > + #cooling-cells = <2>;
> > + /* typical 25 kHz -> 40.000 nsec */
> > + pwms = <&epwm0 1 40000 PWM_POLARITY_INVERTED>;
> > + cooling-levels = <0 32 64 128 196 240>;
> > + pulses-per-revolution = <2>;
> > + interrupt-parent = <&main_gpio1>;
> > + interrupts = <30 IRQ_TYPE_EDGE_FALLING>;
> > + status = "disabled";
>
> please document why disabled?
Will add a comment (disabled as our starterkits don't include a fan, and
different fans may need different PWM frequencies).
>
> [...]
> > +
> > + reg_pwm_fan: regulator-pwm-fan {
> > + compatible = "regulator-fixed";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <®_pwm_fan_pins>;
> > + regulator-name = "FAN_PWR";
> > + regulator-min-microvolt = <12000000>;
> > + regulator-max-microvolt = <12000000>;
>
> Just checking.. Did you intent a 12v supply for fan?
This is a standard 4-pin fan connector as is common on x86 mainboards, which
includes a 12V supply.
>
> > + gpio = <&main_gpio0 62 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + };
> > +
>
> [...]
>
> > +
> > + lvds_panel_pins: lvds-panel-pins {
> > + pinctrl-single,pins = <
> > + /* (N20) GPMC0_BE1n.GPIO0_36 - LVDS0_RESET# */
> > + AM62X_IOPAD(0x0094, PIN_OUTPUT, 7)
>
> Could you follow the comment convention you have elsewhere? please follow the
> same in gpio below etc..
> AM62X_IOPAD(....) /* (N20) ... */
I tried to avoid making the lines overly long an adhered to the regular 100
column limit where reasonable. I agree that the current inconsistent style is
not so nice... I see two options here:
- Consistently move the comment to a separate line
- Break long comments including pin name, pin function and board signal function
into two lines
>
> btw, panel node uses it as:
> enable-gpios = <&main_gpio0 36 GPIO_ACTIVE_HIGH>;
>
> isn't RESET# meant to be active low?
As an "enable" pin enables when active and a "reset" pin disables when active,
an active-low reset is the same as an active-high enable.
>
> > + >;
> > + };
> > +
>
> [...]
>
>
> > diff --git a/arch/arm64/boot/dts/ti/k3-am625-tqma62xx.dtsi
> > b/arch/arm64/boot/dts/ti/k3-am625-tqma62xx.dtsi
> > new file mode 100644
> > index 0000000000000..1b7d58cb7b936
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/ti/k3-am625-tqma62xx.dtsi
> > @@ -0,0 +1,331 @@
> > +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> > +/*
> > + * Copyright (C) 2021-2022 Texas Instruments Incorporated -
> > https://www.ti.com/
> > + * Copyright (c) 2023-2025 TQ-Systems GmbH <[email protected]>,
> > D-82229 Seefeld, Germany.
> > + * Author: Matthias Schiffer
> > + */
>
> [...]
>
> > + regulators {
> > + reg_buck1: buck1 {
> > + regulator-name = "V_VDD_CORE";
> > + regulator-min-microvolt = <750000>;
> > + regulator-max-microvolt = <750000>;
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > +
> [...]
>
> > +
> > +&a53_opp_table {
> > + opp-1400000000 {
> > + opp-hz = /bits/ 64 <1400000000>;
> > + opp-supported-hw = <0x01 0x0004>;
> > + opp-suspend;
> > + clock-latency-ns = <6000000>;
> > + /* Enabled by bootloader if supported */
> > + status = "disabled";
>
> This (bootloader enabling just this node) wont work unless vdd_core
> voltage is configured to 0.8v as well, no?
Correct, the bootloader patches both PMIC config and OPP table.
Best,
Matthias
>
> > + };
> > +};
> > +
>
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
