Re: [PATCH v2 14/19] ARM: dts: Add bus nodes using VDD_INT for Exynos4x12

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On 10.12.2015 16:07, Chanwoo Choi wrote:
> On 2015년 12월 10일 15:53, Krzysztof Kozlowski wrote:
>> On 10.12.2015 15:43, Chanwoo Choi wrote:
>>> On 2015년 12월 10일 15:32, Krzysztof Kozlowski wrote:
>>>> On 10.12.2015 15:08, Chanwoo Choi wrote:
>>>>> On 2015년 12월 10일 14:57, Krzysztof Kozlowski wrote:
>>>>>> On 09.12.2015 13:08, Chanwoo Choi wrote:
>>>>>>> This patch adds the bus noes using VDD_INT for Exynos4x12 SoC.
>>>>>>> Exynos4x12 has the following AXI buses to translate data between
>>>>>>> DRAM and sub-blocks.
>>>>>>>
>>>>>>> Following list specifies the detailed relation between DRAM and sub-blocks:
>>>>>>> - ACLK100 clock for PERIL/PERIR/MFC(PCLK)
>>>>>>> - ACLK160 clock for CAM/TV/LCD
>>>>>>> : The minimum clock of ACLK160 should be over 160MHz.
>>>>>>>   When drop the clock under 160MHz, show the broken image.
>>>>>>> - ACLK133 clock for FSYS
>>>>>>> - GDL clock for LEFTBUS
>>>>>>> - GDR clock for RIGHTBUS
>>>>>>> - SCLK_MFC clock for MFC
>>>>>>>
>>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>> ---
>>>>>>>  arch/arm/boot/dts/exynos4x12.dtsi | 112 ++++++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 112 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
>>>>>>> index 3bcf0939755e..8bc4aee156b5 100644
>>>>>>> --- a/arch/arm/boot/dts/exynos4x12.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/exynos4x12.dtsi
>>>>>>> @@ -354,6 +354,118 @@
>>>>>>>  			opp-microvolt = <950000>;
>>>>>>>  		};
>>>>>>>  	};
>>>>>>> +
>>>>>>> +	bus_leftbus: bus_leftbus {
>>>>>>> +		compatible = "samsung,exynos-bus";
>>>>>>> +		clocks = <&clock CLK_DIV_GDL>;
>>>>>>> +		clock-names = "bus";
>>>>>>> +		operating-points-v2 = <&bus_leftbus_opp_table>;
>>>>>>> +		status = "disabled";
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	bus_rightbus: bus_rightbus {
>>>>>>> +		compatible = "samsung,exynos-bus";
>>>>>>> +		clocks = <&clock CLK_DIV_GDR>;
>>>>>>> +		clock-names = "bus";
>>>>>>> +		operating-points-v2 = <&bus_leftbus_opp_table>;
>>>>>>> +		status = "disabled";
>>>>>>> +	};
>>>>>>
>>>>>> These two nodes are symmetrical. The MFC below and other buses in other
>>>>>> DTS share opps. How about changing the binding so multiple clocks could
>>>>>> be specified at once ("bus0", "bus1")? I think there is no need for a
>>>>>> bus device for each clock.
>>>>>
>>>>> The your commented method is possible.
>>>>>
>>>>> But, I focus on implementing the generic bus frequency driver.
>>>>>
>>>>> If specific bus device-tree node includes the one more clocks,
>>>>> when adding the new Exynos SoC, the exynos-bus.c should be added
>>>>> for new Exynos SoC. Because, each Exynos SoC has the different
>>>>> set of bus device.
>>>>>
>>>>> If we use my approach, we don't need to modify the exynos-bus.c
>>>>> driver to support for the bus frequency of new Exynos SoC.
>>>>
>>>> This won't change. The driver will just support from 1 to N clocks for
>>>> given bus device and set the same OPP to all of them. This will only
>>>> limit the number of duplicated entries. This won't affect the generic
>>>> approach of driver itself.
>>>
>>> You're right aspect of only implementation of device driver.
>>>
>>> But,
>>> If we use your commented approach, we can show the information
>>> of only parent device through sysfs. We cannot see the information
>>> of passive device. The some information includes the current
>>> frequency and correlation of parent device. (But, current patchset
>>> don' include the topology information between parent device and
>>> passive device. I'll do it on later patches).
>>>
>>> For example, 
>>> We can see the following bus device through /sys/class/devfreq.
>>>
>>> drwxr-xr-x  2 root root 0 Dec 31 17:00 .
>>> drwxr-xr-x 44 root root 0 Dec 31 17:00 ..
>>> lrwxrwxrwx  1 root root 0 Dec 31 17:00 bus_display -> ../../devices/platform/bus_display/devfreq/bus_display
>>> lrwxrwxrwx  1 root root 0 Dec 31 17:00 bus_fsys -> ../../devices/platform/bus_fsys/devfreq/bus_fsys
>>> lrwxrwxrwx  1 root root 0 Dec 31 17:00 bus_leftbus -> ../../devices/platform/bus_leftbus/devfreq/bus_leftbus
>>> lrwxrwxrwx  1 root root 0 Dec 31 17:00 bus_peri -> ../../devices/platform/bus_peri/devfreq/bus_peri
>>>
>>>
>>> We don't see the following bus device because of following bus device
>>> has the same frequency table with bus_leftbus device.
>>> lrwxrwxrwx  1 root root 0 Dec 31 17:00 bus_mfc -> ../../devices/platform/bus_mfc/devfreq/bus_mfc
>>> lrwxrwxrwx  1 root root 0 Dec 31 17:00 bus_rightbus -> ../../devices/platform/bus_rightbus/devfreq/bus_rightbus
>>
>> Right, but why do you want to see these bus devices? AFAIU, they will
> 
> I think that the framework should show the accurate information of H/w device
> through sysfs. On the exynos-bus.c driver, it is important the show the
> accurate set of handled bus devices which are included in Exynos SoC.
> 
>> always behave exactly the same as LEFTBUS. Their PPMU counters will be
>> the same... or not? The MFC does not have its own PPMU counter. There
>> are separate counters for left and right bus... but they are attached to
>> the "&bus_leftbus" node. The "&bus_rightbus" does not use the PPMU
>> counter and it follows the parent governor decisions... so this is
>> purely an artificial creation just to handle one clock.
>>
>> I just can't see the benefit of such additional bus device.
> 
> I agree about the behavior. Your description is right.
> There is no difference and benefit about behavior both your and my approach.
> 
> But, We can provide the accurate information of handled bus devices
> to the user-space. I think that it is important information.
> 
> Also, I have the plan that devfreq framework would show
> the topology about the correlation of bus devices as following:

Hmmm.... okay, fair enough. From hardware point of view these are
separate AXI buses so I guess it is good to model them in DT.

Best regards,
Krzysztof