Re: [PATCH v2 5/5] arm64: dts: exynos2200: define all usi nodes

On 8/2/25 03:29, Sam Protsenko wrote:

On Wed, Jul 30, 2025 at 2:44 AM Ivaylo Ivanov
[off-list ref] wrote:

quoted

Universal Serial Interface (USI) supports three types of serial
interfaces - uart, i2c and spi. Each protocol can work independently
and configured using external configuration inputs.

As each USI instance has access to 4 pins, there are multiple possible
configurations:
- the first 2 and the last 2 pins can be i2c (sda/scl) or uart (rx/tx)
- the 4 pins can be used for 4 pin uart or spi

Such configuration can be achieved by setting the mode property of usiX
and usiX_i2c nodes correctly - if usiX is set to take up 2 pins, then
usiX_i2c can be set to take the other 2. If usiX is set for 4 pins, then
usiX_i2c should be left disabled.

The whole naming scheme is a bit confusing: one might think that
because both usiX and usiX_i2c have the same number (X), they
represent the same USI block.

Mapped to a different address? Hm, I doubt.

 I can see how they might share the same
pins, but it doesn't seem enough to me to justify this convention. If
I'm missing something, please help me understand why it should be done
like that?

That's the way it was done by Samsung in downstream, and specifically in
upstream for autov9 [1]. Clocks are already merged and definitions expect
the convention implied by my patch. Iit'll be way more mangled if we use
non-matching node to header-definition names.

quoted

Define all the USI nodes from peric0 (usi4), peric1 (usi7-10), peric2
(usi0-6, usi11) and cmgp (usi0-6_cmgp, 2 pin usi7_cmgp) blocks, as well
as their respective uart and i2c subnodes. As Samsung, for some reason,
has decided to restart the counting of usi instances for cmgp, suffix
labels for nodes of such with _cmgp.

Yeah, they probably meant to number CMGP instances, not USI instances.
Because CMGP (stands for Common GPIO) is actually a separate IP block
containing:
  - 2 x USIs
  - 1 GPIO controller (8 GPIO lines)
  - One general purpose ADC
  - 6 interrupt combiners

So some USI blocks are separate USIs, and some USI blocks are a part
of bigger CMGP blocks. And instead of using "usi_01_cmgp" for example,
they should've gone with "usi_cmgp01".

I guess.

Usually it's recommended to follow the naming scheme from the TRM, but
AFAIU you don't have one.

Yes...

 And the scheme used in the downstream device
tree looks like comlete garbage. Anyways, I don't have strong
preference on the naming scheme. Frankly I'd just do the consecutive
numbering for all the USI nodes in this case, like: usi0, usi1, etc.
And add the comments when needed, like "USI from CMGP01 block".

This will mangle stuff way too much for my preference. People will be
way more confused when comparing this to the downstream device tree
too, as the average Jo does not have access to anything apart from kernel
source (like me -_-).

quoted

Spi support will be added later on.

Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
---
 arch/arm64/boot/dts/exynos/exynos2200.dtsi | 1361 ++++++++++++++++++++
 1 file changed, 1361 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/exynos2200.dtsi b/arch/arm64/boot/dts/exynos/exynos2200.dtsi
index 22c6da907..f83e6cf24 100644
--- a/arch/arm64/boot/dts/exynos/exynos2200.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos2200.dtsi

@@ -7,6 +7,7 @@

 #include <dt-bindings/clock/samsung,exynos2200-cmu.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/soc/samsung,exynos-usi.h>

 / {
        compatible = "samsung,exynos2200";

@@ -314,6 +315,76 @@ pinctrl_peric0: pinctrl@10430000 {
                        reg = <0x10430000 0x1000>;
                };

+               usi4: usi@105000c0 {
+                       compatible = "samsung,exynos2200-usi", "samsung,exynos850-usi";
+                       reg = <0x105000c0 0x20>;
+                       ranges;
+                       #address-cells = <1>;
+                       #size-cells = <1>;
+                       clocks = <&cmu_peric0 CLK_MOUT_PERIC0_NOC_USER>,
+                                <&cmu_peric0 CLK_DOUT_PERIC0_USI04>;
+                       clock-names = "pclk", "ipclk";
+                       samsung,sysreg = <&syscon_peric0 0x1024>;
+                       status = "disabled";
+
+                       hsi2c_8: i2c@10500000 {

Why not number all the underlying protocol nodes using the same number
as the USI node?

Same as above, my wild guess is that Samsung ordered them by physical
position.

Best regards,
Ivaylo

[1] https://github.com/torvalds/linux/blob/cca7a0aae8958c9b1cd14116cb8b2f22ace2205e/arch/arm64/boot/dts/exynos/exynosautov9.dtsi#L503

 Like it's done in gs101.dtsi. And maybe even follow
USI naming scheme used in gs101 in general? Like, sort all USI nodes
by unit address, and then number them starting from 0. If some other
USIs are missing (like I mentioned in my review for the previous
patch), add those too, first.

[snip]