Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation
 for interconnect properties
To:     Georgi Djakov <georgi.djakov@linaro.org>,
        Rob Herring <robh@kernel.org>
Cc:     cw00.choi@samsung.com, krzk@kernel.org, devicetree@vger.kernel.org,
        a.swigon@samsung.com, myungjoo.ham@samsung.com,
        inki.dae@samsung.com, sw0312.kim@samsung.com,
        b.zolnierkie@samsung.com, m.szyprowski@samsung.com,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-samsung-soc@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org
From:   Sylwester Nawrocki <s.nawrocki@samsung.com>
Message-ID: <e6e369fb-ccf2-09ed-ad6a-680e67198359@samsung.com>
Date:   Wed, 9 Sep 2020 16:47:06 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
        Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <b711257d-c34b-b609-3ada-312871967b98@linaro.org>
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20200702163724.2218-1-s.nawrocki@samsung.com>
        <CGME20200702163748eucas1p2cf7eab70bc072dea9a95183018b38ad3@eucas1p2.samsung.com>
        <20200702163724.2218-2-s.nawrocki@samsung.com>
        <20200709210448.GA876103@bogus>
        <65af1a5c-8f8a-ef65-07f8-e0b3d04c336c@samsung.com>
        <35d9d396-b553-a815-1f3b-1af4dc37a2ca@samsung.com>
        <b711257d-c34b-b609-3ada-312871967b98@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Georgi,

On 09.09.2020 11:07, Georgi Djakov wrote:
> On 8/28/20 17:49, Sylwester Nawrocki wrote:
>> On 30.07.2020 14:28, Sylwester Nawrocki wrote:
>>> On 09.07.2020 23:04, Rob Herring wrote:
>>>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>>>> Add documentation for new optional properties in the exynos bus nodes:
>>>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>>>> These properties allow to specify the SoC interconnect structure which
>>>>> then allows the interconnect consumer devices to request specific
>>>>> bandwidth requirements.
>>>>>
>>>>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>>>>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt

>>>>> +Optional properties for interconnect functionality (QoS frequency constraints):
>>>>> +- samsung,interconnect-parent: phandle to the parent interconnect node; for
>>>>> +  passive devices should point to same node as the exynos,parent-bus property.
>>
>>>> Adding vendor specific properties for a common binding defeats the 
>>>> point.
>>
>> Actually we could do without any new property if we used existing interconnect
>> consumers binding to specify linking between the provider nodes. I think those
>> exynos-bus nodes could well be considered both the interconnect providers 
>> and consumers. The example would then be something along the lines 
>> (yes, I know the bus node naming needs to be fixed):
>>
>> 	soc {
>> 		bus_dmc: bus_dmc {
>> 			compatible = "samsung,exynos-bus";
>> 			/* ... */
>> 			samsung,data-clock-ratio = <4>;
>> 			#interconnect-cells = <0>;
>> 		};
>>
>> 		bus_leftbus: bus_leftbus {
>> 			compatible = "samsung,exynos-bus";
>> 			/* ... */
>> 			interconnects = <&bus_leftbus &bus_dmc>;
>> 			#interconnect-cells = <0>;
>> 		};
>>
>> 		bus_display: bus_display {
>> 			compatible = "samsung,exynos-bus";
>> 			/* ... */
>> 			interconnects = <&bus_display &bus_leftbus>;
> 
> Hmm, bus_display being a consumer of itself is a bit odd? Did you mean:
>  			interconnects = <&bus_dmc &bus_leftbus>;

Might be, but we would need to swap the phandles so <source, destination>
order is maintained, i.e. interconnects = <&bus_leftbus &bus_dmc>;

My intention here was to describe the 'bus_display -> bus_leftbus' part 
of data path 'bus_display -> bus_leftbus -> bus_dmc', bus_display is
really a consumer of 'bus_leftbus -> bus_dmc' path.

I'm not sure if it is allowed to specify only single phandle (and 
interconnect provider specifier) in the interconnect property, that would
be needed for the bus_leftbus node to define bus_dmc as the interconnect 
destination port. There seems to be such a use case in arch/arm64/boot/
dts/allwinner/sun50i-a64.dtsi. 

>> 			#interconnect-cells = <0>;
>> 		};
>>
>>
>> 		&mixer {
>> 			compatible = "samsung,exynos4212-mixer";
>> 			interconnects = <&bus_display &bus_dmc>;
>> 			/* ... */
>> 		};
>> 	};
>>
>> What do you think, Georgi, Rob?
> 
> I can't understand the above example with bus_display being it's own consumer.
> This seems strange to me. Could you please clarify it?

> Otherwise the interconnect consumer DT bindings are already well established
> and i don't see anything preventing a node to be both consumer and provider.
> So this should be okay in general.

Thanks, below is an updated example according to your suggestions. 
Does it look better now?

---------------------------8<------------------------------
soc {
	bus_dmc: bus_dmc {
		compatible = "samsung,exynos-bus";
		/* ... */
		samsung,data-clock-ratio = <4>;
		#interconnect-cells = <0>;
	};

	bus_leftbus: bus_leftbus {
		compatible = "samsung,exynos-bus";
		/* ... */
		interconnects = <&bus_dmc>;
		#interconnect-cells = <0>;
	};

	bus_display: bus_display {
		compatible = "samsung,exynos-bus";
		/* ... */
		interconnects = <&bus_leftbus &bus_dmc>;
		#interconnect-cells = <0>;
	};

	&mixer {
		compatible = "samsung,exynos4212-mixer";
		interconnects = <&bus_display &bus_dmc>;
		/* ... */
	};
};
---------------------------8<------------------------------

-- 
Regards,
Sylwester