Re: [PATCH v2 2/2] arm64: dts: mediatek: add Kontron 3.5"-SBC-i1200

From: AngeloGioacchino Del Regno
Date: Mon Feb 19 2024 - 08:35:38 EST

Il 19/02/24 14:09, Michael Walle ha scritto:
Hi,

thanks for the extensive review!

On Mon Feb 19, 2024 at 11:00 AM CET, AngeloGioacchino Del Regno wrote:

+&eth {
+ phy-mode ="rgmii-id";
+ phy-handle = <&ethernet_phy0>;
+ snps,reset-gpio = <&pio 93 GPIO_ACTIVE_HIGH>;
+ snps,reset-delays-us = <0 10000 80000>;

snps,reset-delays-us and snps,reset-gpio are deprecated.

+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&eth_default_pins>;
+ pinctrl-1 = <&eth_sleep_pins>;
+ status = "okay";
+
+ mdio {
+ ethernet_phy0: ethernet-phy@1 {

compatible = "is there any applicable compatible?"
P.S.: if you've got the usual rtl8211f, should be "ethernet-phy-id001c.c916"

I'd rather not have a compatible here. First, it's auto discoverable
and IIRC it's frowned upon adding any compatible if you ask the PHY
maintainers. And second, if we change the PHY (maybe due to a second
chip shortage or whatever), there is a chance you don't have to
update this in the DT.


Okay then, I'm fine with leaving the compatible out.

reg = <0x1>;
interrupts-extended = <&pio 94 IRQ_TYPE_LEVEL_LOW>;
reset-assert-us = <10000>;
reset-deassert-us = <80000>;
reset-gpios = <&pio 93 GPIO_ACTIVE_HIGH>;


+ reg = <0x1>;
+ interrupts-extended = <&pio 94 IRQ_TYPE_LEVEL_LOW>;
+ };
+ };
+};
+
+&gpu {
+ status = "okay";
+ mali-supply = <&mt6315_7_vbuck1>;
+};
+
+&i2c2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c2_pins>;
+ clock-frequency = <400000>;
+ status = "okay";

Are i2c2,3,4 exposed as pins somewhere? If they are, can you please put a
comment saying so?

This is only a basic device tree. On one i2c controller, there is
the LVDS bridge for example. My plan is to get the support for this
bridge upstream first and then adding the appropriate device nodes
here.

That being said, some are exposed to connectors. I'll add a comment
then.

In that case, could be nice to read something like

&i2c(x) {
properties
blahblah
status

/* (model, if available) LVDS bridge at 0x10 */
}

it's again not mandatory, but I like seeing clear messages implying "this should be
there" as those implicitly mean "...yeah but it's not supported yet for reasons".

It's down to preferences though, and this is not a *strong* opinion, nor a strong
suggestion - your call here.


+&mmc1 {
+ pinctrl-names = "default", "state_uhs";
+ pinctrl-0 = <&mmc1_default_pins>;
+ pinctrl-1 = <&mmc1_uhs_pins>;
+ cd-gpios = <&pio 129 GPIO_ACTIVE_LOW>;
+ bus-width = <4>;
+ max-frequency = <200000000>;
+ cap-sd-highspeed;
+ sd-uhs-sdr50;
+ sd-uhs-sdr104;
+ vmmc-supply = <&mt6360_ldo5>;
+ vqmmc-supply = <&mt6360_ldo3>;

Does mmc1 support eMMC and SDIO?

No eMMC, but I'd guess it will support SDIO as in you can just plug
an SDIO card in the SD slot, right? Oh, it's a micro SD socket. So
uhm, I'm not sure if we should restrict it, though. Someone might
come up with a microsd to sd card adapter. I have one right in front
of me ;)


Honestly ... I even forgot the existance of those adapters!!!
In that case, yes, since the controller should support SDIO on that slot, and since
there effectively are ways to add a SDIO card on there, obviously no-sdio shall be
omitted.

I agree.

If not, no-mmc; no-sdio;

So no-mmc;

Yes, agreed.


+ drive-strength = <MTK_DRIVE_8mA>;

s/MTK_DRIVE//g
s/mA//g

drive-strength = <8>;

Please, here and everywhere else, for all values - let's stop using those
MTK_DRIVE_(x)mA definitions, they're just defined as (x), where anyway
the drive-strength property is in milliamps by default.

We don't need these definitions.

Sure, the mt8195-demo was the blueprint for this. So maybe you should
get rid of it there to prevent any copying ;) (btw the same goes for
the regulator-compatible property).


Yeah, that's right. You can imagine that my backlog is rather huge... :-)

Speaking of pinctrl, I find the R0R1 bias-pull-down values really

If it was only pull-down it would be one problem, but it's also pull-up so
we can sum that up to *two* problems :-P

hard to grasp. The DT binding documentation didn't really help here.
What is R0 and R1, I presume some resistors which can be enabled.

You got it right

Also are they in parallel or in series. I'd have assumed, the DT

I'm not sure, and it depends on the SoC most probably... but does that really
matter?

I mean, on the practical side, imo, it doesn't, but I am also a curious person
so I can understand why you're eager to know :-)

binding should have hid this by giving the user a choice for the
resistance instead. Also I had a quick search in the RM and
couldn't find anything, I probably looked at the wrong place ;)


I'm not sure you looked at mediatek,mt8195-pinctrl.yaml, but anyway, as you
can read in there, we're deprecating the MTK_PULL_SET_RSEL_xxx in favor of...

... the right thing to do :-)

Look for "mediatek,rsel-resistance-in-si-unit": that'll allow you to specify
the PU/PD values in ohms, and that's what should be used.

Those RSEL definitions in the devicetree should disappear. Forever.

+ uart1_pins: uart1-pins {
+ pins_rx {
+ pinmux = <PINMUX_GPIO103__FUNC_URXD1>;
+ input-enable;
+ bias-pull-up;
+ };
+
+ pins_tx {
+ pinmux = <PINMUX_GPIO102__FUNC_UTXD1>;
+ };
+
+ pins_rts {
+ pinmux = <PINMUX_GPIO100__FUNC_URTS1>;
+ output-enable;

Are you really sure that you need output-enable here?!
RTS is not an output buffer....

I don't think you do. Please double check.

Ahh, good catch, it's a leftover from mt8183-kukui.dts. There is
probably wrong, too.


Probably. I don't really know either.

+ };
+
+ pins_cts {
+ pinmux = <PINMUX_GPIO101__FUNC_UCTS1>;
+ input-enable;
+ };
+ };
+


+/* USB3 front port */
+&xhci0 {

It's not gonna work like this. I recently fixed the USB nodes in MT8195 by adding
MTU3 where necessary...

Uhm, seems like I've missed that.


No worries!

Check mt8195.dtsi - only one XHCI controller isn't placed behind MTU3, and that is
XHCI1 (11290000), while the others are MTU3.

As far as I can see from this DT, it should now instead look like..

&ssusb0 {
dr_mode = "host";
vusb33-supply = <&mt6359_vusb_ldo_reg>;
status = "okay";
};

&ssusb2 {
dr_mode = "host";
vusb33-supply = <&mt6359_vusb_ldo_reg>;
status = "okay";
};

&ssusb3 {
dr_mode = "host";
vusb33-supply = <&mt6359_vusb_ldo_reg>;
status = "okay";
};

&xhci0 {
vbus-supply = <&otg_vbus_regulator>;
status = "okay";
};

&xhci1 {
vusb33-supply = <&mt6359_vusb_ldo_reg>;

vbus is always supplied by something, as otherwise USB won't work - whether this
is an always-on regulator or a passthrough from external supply this doesn't really
matter - you should model a regulator-fixed that provides the 5V VBUS line.

I don't think this is correct, though. Think of an on-board USB
hub. There only D+/D- are connected (and maybe the USB3.2 SerDes
lanes). Or have a look at the M.2 pinout. There is no Vbus.


Yes but the MediaTek MTU3 and/or controllers do have it ;-)

Also it seems I need the "mediatek,u3p-dis-msk = <0x01>;". At least
the last time I've tested it. I'll test it again, with and without.
The SerDes Line of the corresponding USB3.2 port is used for PCIe in
this case.


Have I missed it in my example? If I missed it, that was unintentional.

Anyway, for the u3p-dis-msk, I'll spare you the time to check:
- If the controller lies behind MTU3, that property goes to &ssusb(x)
- If it is a standalone XHCI controller, it goes to &xhci(x)
- The property never goes to both, and always goes to the *outer* node
(this is why it goes to mtu3 if there's a mtu3 behind).

For example:
vbus_fixed: regulator-vbus {
compatible = "regulator-fixed";
regulator-name = "usb-vbus";
regulator-always-on;
regulator-boot-on;
regulator-min-microvolt = <5000000>;
regulator-max-microvolt = <5000000>;
};

As mentioned above, I don't think this will make sense in my case.
>> P.S.: If the rail has a different name, please use that different name. Obviously
that requires you to have schematics at hand, and I don't know if you do: if you
don't, then that regulator-vbus name is just fine.

I do have the schematics.

In that case, you should model the power tree with the fixed power lines,
check mt8195-cherry (and/or cherry-tomato) and radxa-nio-12l; even though
those are technically "doing nothing", this is device tree, so it should
provide a description of the hardware ... and the board does have fixed
power lines.
It has at least one: DC-IN (typec, barrel jack or whatever, the board needs
power, doesn't it?!).

Cheers,
Angelo