Re: [PATCH 2/3] arm64: dts: exynosautov9: add exynosautov9-usi.dtsi

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 29/06/2022 11:47, Chanho Park wrote:
>> On 29/06/2022 03:56, Chanho Park wrote:
>>> Universal Serial Interface (USI) supports three types of serial
>>> interface such as Universal Asynchronous Receiver and Transmitter
>>> (UART), Serial Peripheral Interface (SPI), and Inter-Integrated Circuit
>> (I2C).
>>> Each protocols can be working independently and configured as one of
>>> those using external configuration inputs.
>>> Exynos Auto v9 SoC support 12 USIs. When a USI uses two pins such as
>>> i2c and 3 wire uarts(RX/TX only), we can use remain two pins as i2c mode.
>>> So, we can define one USI node that includes serial/spi and hsi2c.
>>> usi_i2c nodes can be used only for i2c mode.
>>>
>>> We can have below combinations for one USI.
>>> 1) The usi node is used either 4 pin uart or 4 pin spi  -> No usi_i2c
>>> can be used
>>> 2) The usi node is used 2 pin uart(RX/TX) and i2c(SDA/SCL)  -> usi_i2c
>>> should be enabled to use the latter i2c
>>> 3) The usi node is used i2c(SDA/SCL) and i2c(SDA/SCL)  -> usi_i2c
>>> should be enabled to use the latter i2c
>>>
>>> By default, all USIs are initially set to uart mode by below setting.
>>> samsung,mode = <USI_V2_UART>;
>>> You can change it either USI_V2_SPI or USI_V2_I2C.
>>>
>>> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
>>> ---
>>>  .../boot/dts/exynos/exynosautov9-usi.dtsi     | 1127 +++++++++++++++++
>>>  1 file changed, 1127 insertions(+)
>>>  create mode 100644 arch/arm64/boot/dts/exynos/exynosautov9-usi.dtsi
>>
>> Put all this directly in exynosautov9.dtsi, because this is not a re-
>> usable piece among different DTSI.
> 
> Okay. I'll move them in the exynosautov9.dtsi. I thought they're too long to put in the exynosautov9.dtsi and I also found a similar case such as exynos5433-bus.dtsi and exynos5433-tmu.dtsi.

Indeed we did like that... The tmu maybe was meant to be re-used,
although it references specific clusters. But the split of bus I don't
understand - it does not help.

I don't think it improved readability.

> 
>>
>>>
>>> diff --git a/arch/arm64/boot/dts/exynos/exynosautov9-usi.dtsi
>>> b/arch/arm64/boot/dts/exynos/exynosautov9-usi.dtsi
>>> new file mode 100644
>>> index 000000000000..0e4c6332770b
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/exynos/exynosautov9-usi.dtsi
>>> @@ -0,0 +1,1127 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Samsung's ExynosAutov9 SoC USI device tree source
>>> + *
>>> + * Copyright (c) 2022 Samsung Electronics Co., Ltd.
>>> + *
>>> + * Samsung's ExynosAutov9 SoC USI(Universal Serial Interface,
>>> +uart/spi/i2c)
>>> + * are listed as device tree nodes in this file.
>>> + */
>>> +
>>> +/* PERIC0 USIs */
>>> +&soc {
>>> +	syscon_peric0: syscon@10220000 {
>>> +		compatible = "samsung,exynosautov9-sysreg", "syscon";
>>> +		reg = <0x10220000 0x2000>;
>>> +	};
>>> +
>>> +	usi_0: usi@103000c0 {
>>> +		compatible = "samsung,exynos850-usi";
>>
>> We should start adding dedicated compatible, so:
>> "samsung,exynosautov9-usi", "samsung,exynos850-usi"
> 
> So, I need to add the compatible to the Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml, right?
> 
> - samsung,exynos850-usi   # for USIv2 (Exynos850, ExynosAutoV9)
> 
> To be>
> - samsung,exynos850-usi
> - samsung,exynosautov9-usi


  compatible:

    oneOf:
     - items:
         - const: samsung,exynosautov9-usi
         - const: samsung,exynos850-usi
     - enum:

         - samsung,exynos850-usi   # for USIv2 (Exynos850, ExynosAutoV9)

> 
>>
>>> +		reg = <0x103000c0 0x20>;
>>> +		samsung,sysreg = <&syscon_peric0 0x1000>;
>>> +		samsung,mode = <USI_V2_UART>;
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		ranges;
>>> +		clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PCLK_0>,
>>> +			 <&cmu_peric0 CLK_GOUT_PERIC0_IPCLK_0>;
>>> +		clock-names = "pclk", "ipclk";
>>> +		status = "disabled";
>>> +
>>> +		/* USI: UART */
>>
>> Skip the comments, they are obvious from device node name. Long time ago I
>> was not advocating this, but I see it's benefits - much easier to
>> introduce changes to DTS or binding in case of some differences.

Eh, I think my reply got mixed up. The last sentence was about
compatible, so it should be:

We should start adding dedicated compatible, so:
"samsung,exynosautov9-usi", "samsung,exynos850-usi".
Long time ago I was not advocating this, but I see it's benefits - much
easier to introduce changes to DTS or binding in case of some differences.

and here only about the comment.

> I'll drop them.

Yes, please.

> 
>>
>>> +		serial_0: serial@10300000 {
>>> +			compatible = "samsung,exynos850-uart";
>>
>> Here as well.
> 
> I'll add "samsung,exynosautov9-uart" to the yaml file.
> 
>>

Best regards,
Krzysztof