Re: [PATCH v8 02/20] PM / devfreq: exynos: Add documentation for generic exynos bus frequency driver

[Chanwoo Choi]

Hi Rob,

On 2016ë 04ì 12ì 05:25, Chanwoo Choi wrote:
> Hi Rob,
> 
> On Tue, Apr 12, 2016 at 12:40 AM, Rob Herring <robh@xxxxxxxxxx> wrote:
>> On Fri, Apr 08, 2016 at 01:24:51PM +0900, Chanwoo Choi wrote:
>>> This patch adds the documentation for generic exynos bus frequency
>>> driver.
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
>>> Signed-off-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> ---
>>>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 95 ++++++++++++++++++++++
>>>  1 file changed, 95 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>> new file mode 100644
>>> index 000000000000..78171b918e3f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>> @@ -0,0 +1,95 @@
>>> +* Generic Exynos Bus frequency device
>>> +
>>> +The Samsung Exynos SoC has many buses for data transfer between DRAM
>>> +and sub-blocks in SoC. Most Exynos SoCs share the common architecture
>>> +for buses. Generally, each bus of Exynos SoC includes a source clock
>>> +and a power line, which are able to change the clock frequency
>>> +of the bus in runtime. To monitor the usage of each bus in runtime,
>>> +the driver uses the PPMU (Platform Performance Monitoring Unit), which
>>> +is able to measure the current load of sub-blocks.
>>> +
>>> +There are a little different composition among Exynos SoC because each Exynos
>>> +SoC has different sub-blocks. Therefore, shch difference should be specified
>>> +in devicetree file instead of each device driver. In result, this driver
>>> +is able to support the bus frequency for all Exynos SoCs.
>>
>> I still have issues with this whole series. The DT hierarchy represents
>> buses. You are describing buses here and control of them. I would expect
>> to see some hierarchy, but there is none. What this looks like is you
>> are adding nodes based on what fits the current driver.
> 
> I already replied[1] about your same question on v2 patchset four 4 months ago.
> [1] https://lkml.org/lkml/2015/12/10/943
> 
> I attach the your question and my reply history as following:
> ---------------------------------------------------------------------
> [ Discussion between you and me on v2 patchset[1] ]
>>>>
>>>> This still has the same problem as before. I would expect that the bus
>>>> hierarchy in the dts match the hierarchy here. You just have flat nodes
>>>> in the example below. So all IP blocks affected by frequency scaling
>>>> should be under the bus node defining the OPPs. Something like this:
>>>
>>> The each bus of sub-block has not h/w dependency among sub-blocks
>>> and has the owned source clock / OPP table. Just they share the same
>>> power line. So, I think that flat nodes in the example below is not problem.
>>
>> I'm talking about the peripherals not described here. Is the ISP block
>> not a child of the bus_isp node? Same for the display controller block
>> and bus_lcd0. And so on.
> 
>>From the H/W point of view, ISP block is really not included in ISP's
> AXI bus (bus_isp).
> Just, the bus_isp connect to between ISP block and DRAM.
> ---------------------------------------------------------------------

I'm considering about your comment about hierarchy.

But, I think it is not appropriate.
there is no perfect paired set between sub-block and dvfs driver for AMBA bus.

For example about Exynos5420's JPEG IP,
There are two JPEG IPs in the Exynos5420. But two JPEG IPs
use the same source clock for AMBA AXI line to transfer
data between DRAM and JPEG as following:

(AXI master)               (AXI slave)
JPEG_0 --|-----(AXI)-----|
         |               |---- DRAM
JPEG_1 --|-----(AXI)-----|

Two AMBA buses use the same source clock. So, only one dvfs driver
should handle the source clock for AXI bus.
               
Also,
two JPEG use the same AMBA APB line to configure the register of JPEG IP
as following:

(APB slave)                 (APB master)
JPEG_0 --|-----(APB)-----|
         |               |---- CPU
JPEG_1 --|-----(APB)-----|

The JPEG_0/1 could not be included in both AXI and APB in Device Tree.

Additionally, the I2C/SPI/PCI and etc use the APB bus
for communication between each controller and CPU.

I'll expect your reply.

Best Regards,
Chanwoo Choi