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

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6566 bytes --]

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.

> 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.

> > +&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 ;)

> If not, no-mmc; no-sdio;

So no-mmc;

> > +			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).

Speaking of pinctrl, I find the R0R1 bias-pull-down values really
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.
Also are they in parallel or in series. I'd have assumed, the DT
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 ;)

> > +	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.

> > +		};
> > +
> > +		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.

> 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.

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.

> 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.

Thanks,
-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 252 bytes --]