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@xxxxxxxxxxx>
>>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>>
>>>>> --- 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
