Message-ID: <b50d49fd-2976-46fc-9f35-354fb39720ad@collabora.com>
Date: Mon, 19 Feb 2024 14:35:26 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Michael Walle <mwalle@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>
Cc: devicetree@...r.kernel.org, Sean Wang <sean.wang@...iatek.com>,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2 2/2] arm64: dts: mediatek: add Kontron 3.5"-SBC-i1200

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

Powered by blists - more mailing lists